Declarative CSS Modules#11687
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces declarative CSS modules support to HTML, allowing CSS to be imported as modules through <style> elements with a specifier attribute and <template> elements with a shadowrootadoptedstylesheets attribute.
- Adds a
specifierattribute to<style>elements that creates module import maps for CSS content - Introduces a
shadowrootadoptedstylesheetsattribute for<template>elements to declaratively adopt CSS modules - Implements algorithms for creating declarative CSS module scripts and stylesheet adoption
Comments suppressed due to low confidence (5)
source:1
- Missing attribute name in IDL definition. Should be
[SameObject, PutForwards=value, Reflect] readonly attribute DOMString <dfn attribute for="HTMLStyleElement" data-x="dom-style-specifier">specifier</dfn>;to match the pattern used for other attributes in this interface.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Grammar error: 'is defines' should be 'defines' - remove the word 'is'.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Grammar error: 'appended with the of the' should be 'appended with the' - remove the duplicate 'the of'.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Logic error: The algorithm references
<var>specifier</var>but this variable is not defined in the algorithm. It should reference the value of theshadowrootadoptedstylesheetsattribute instead.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference: Should be
data-x=\"attr-style-specifier\"notdata-x=\"attr-style-blocking\"for the specifier attribute row.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
source:1
- The IDL attribute should be named to match the content attribute. The content attribute is
specifierbut the IDL should use camelCase convention:[SameObject, PutForwards=value, Reflect] readonly attribute DOMString specifier;should have a data-x attribute defining the DOM property name.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The
shadowrootadoptedstylesheetsattribute is listed twice in the content attributes section for the template element. This duplication should be removed.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The
shadowrootadoptedstylesheetsattribute is listed twice in the content attributes section for the template element. This duplication should be removed.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The algorithm step is missing proper HTML structure. It should end with
</li>and the nested<ol>should be properly closed with</ol>.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- The variable
moduleScriptis referenced but never defined in this algorithm. This should likely be the current module script or settings object context.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- These variable assignments are missing closing
</p>tags. Each should end with</p></li>.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Missing spaces after commas in the parameter list. Should be
<var>fetchClient</var>, <var>destination</var>, <var>options</var>, <var>settingsObject</var>, <var>referrer</var>for consistency.
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference in the table. Line 148289 should reference
data-x=\"element-template\"or similar, notdata-x=\"attr-template-shadowrootclonable\".
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
source:1
- Incorrect data-x reference in the table. Line 148363 should reference
data-x=\"element-style\"instead ofdata-x=\"attr-style-blocking\".
<!-- -*- mode: Text; fill-column: 100 -*- vim: set textwidth=100 :
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dandclark
left a comment
There was a problem hiding this comment.
Review in progress, sharing feedback so far.
mhochk
left a comment
There was a problem hiding this comment.
LGTM (No permissions to actually Approve)
dandclark
left a comment
There was a problem hiding this comment.
This is coming together nicely. I think the biggest thing still to figure out is to prevent a given <style type=module> from being processed twice. I'm about to head out on leave so I'm going to Approve this since I'm supportive of the direction and I trust that the remaining open issues will be handled appropriately.
| <ol> | ||
| <li><p>Let <var>element</var> be the <code>style</code> element.</p></li> | ||
|
|
||
| <li><p>If <var>element</var> is not <span>connected</span>, then return.</p></li> |
There was a problem hiding this comment.
I guess this is also the point where we'd also check a new equivalent of the already started flag to ensure a given <style type=module> only ever gets processed once?
There was a problem hiding this comment.
Yeah I'll have to think about this a little more. I will likely bring this up soon with the WHATWG.
There was a problem hiding this comment.
It doesn't seem like this got addressed one way or another?
| of the value of the <span data-x="attr-style-specifier">specifier attribute</span> and a value of | ||
| <var>styleDataURI</var>.</p></li> | ||
|
|
||
| <li><p><span>Create an import map parse result</span> with <var>input</var> as <var>jsonString</var> |
There was a problem hiding this comment.
Create an import map parse result can throw -- do we need to handle any of those cases? The one I particularly had in mind to watch for is does it throw when a given specifier is invalid? If not, do we need to handle an invalid specifier some other way?
There was a problem hiding this comment.
Good question. For now I added a step to look at the importMapParseResult's error to rethrow and to continue if that happens. In practice, we probably want to log something in the console, but this might be enough for the spec.
|
You can include "Fixes #10673" in the OP so that there's a link from the issue in GitHub's UI. (Also should be in the commit message when squashing.) |
|
@annevk - I believe the last two commits addressed all of the issues you brought up this morning, so please take a look when you get a chance. And also let me know if I missed anything else you mentioned today. |
|
I think the most substantive issue I raised was not addressed: #10673 (comment). I think we either need something like |
@annevk - I just pushed an update that switches the dataURI to a Blob URI. I think this approach is much simpler than expanding the module map to contain element references. I've been thinking about the Blob approach and haven't come up with any major reasons not to go this route. The lifetime of the Blob object is an interesting one, and it would be nice to give developers a way to revoke the Blob URL. I don't think removing the |
|
@keithamus - here is the split PR with just the declarative modules |
|
I still think that we can have declarative CSS modules with URLs before we go down the rabbit hole of supporting them in inline styles with something like The main issue with re-importing CSS URLs in shadow DOM is that they are duplicate. <script type=importmap>
{ "mytheme": { type: "css", href: "theme.css" } }
</script>
<my-element>
<template shadowrootadoptedstylesheets="mytheme">...</theme>
</my-element>Or something like: <script type=importmap>
{ "mytheme": { type: "css", href: "theme.css" } }
</script>
<my-element>
<link rel=stylesheetmodule href="theme.css">
<!-- or -->
<link rel=stylesheetmodule href="mytheme">
</my-element>This decouples the issue of deduping stylesheet references from the issue of importing inline styles. |
Thanks @noamr. I've recently split the URL version into its own explainer, see https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/ShadowDOMAdoptedStyleSheets/explainer.md.
This proposal creates an import map entry under-the-hood for the |
annevk
left a comment
There was a problem hiding this comment.
This still looks far from ready to me. I'm also wondering if we have a good story as to how we eventually want to make this consistent with the script element. Will that get a specifier attribute too?
| attribute boolean <span data-x="dom-style-disabled">disabled</span>; | ||
| [<span>CEReactions</span>, <span data-x="xattr-Reflect">Reflect</span>] attribute DOMString <dfn attribute for="HTMLStyleElement" data-x="dom-style-media">media</dfn>; | ||
| [SameObject, PutForwards=<span data-x="dom-DOMTokenList-value">value</span>, <span data-x="xattr-Reflect">Reflect</span>] readonly attribute <span>DOMTokenList</span> <dfn attribute for="HTMLStyleElement" data-x="dom-style-blocking">blocking</dfn>; | ||
| [SameObject, PutForwards=value, Reflect] readonly attribute DOMString specifier; |
|
|
||
| <ul> | ||
| <li><p>The element is popped off the <span>stack of open elements</span> of an <span>HTML | ||
| parser</span> or <span>XML parser</span>.</p></li> |
There was a problem hiding this comment.
Is this just a subset of the next condition?
| <code data-x="dom-Blob-type">type</code> of "<code>text/css</code>".</p></li> | ||
|
|
||
| <li><p>Let <var>styleBlobURL</var> be the <span data-x="concept-url-blob-entry">blob URL entry</span> | ||
| associated with <var>styleBlob</var>.</p></li> |
There was a problem hiding this comment.
This is not how this works. A Blob isn't automatically in a store and concept-url-blob-entry doesn't belong to Blob objects either. (Also, a blob URL entry is a struct, not a URL.)
| <var>styleBlobURL</var>.</p></li> | ||
|
|
||
| <li><p>Let <var>jsonString</var> be the result of calling <span>JSON.stringify</span> on | ||
| <var>jsonObject</var>.</p></li> |
There was a problem hiding this comment.
This is wrong. We should be using Infra primitives for JSON.
| <p>Authors should not specify a value for the <code data-x="attr-style-type">type</code> | ||
| attribute on <code>style</code> elements, if that value is an <span>ASCII case-insensitive</span> | ||
| match for "<code>text/css</code>". Instead, they should omit the attribute, which has the same | ||
| effect.</p> |
There was a problem hiding this comment.
This is wrong. Now we're no longer stating what the attribute must contain.
| <ol> | ||
| <li><p>Let <var>element</var> be the <code>style</code> element.</p></li> | ||
|
|
||
| <li><p>If <var>element</var> is not <span>connected</span>, then return.</p></li> |
There was a problem hiding this comment.
It doesn't seem like this got addressed one way or another?
| with <var>input</var> as <var>jsonString</var> and <var>baseURL</var> as the <span>document base URL</span>.</p></li> | ||
|
|
||
| <li><p>If <var>importMapParseResult</var>'s <span data-x="impr-error-to-rethrow">error to rethrow</span> is | ||
| not null, then return.</p></li> |
There was a problem hiding this comment.
Why would we silently eat exceptions? Can't we let register an import map surface them?
|
I think that the main issue with this solution, beyond it being tailor-made for styles and ignored for scripts, is that "shadow roots are able to export styles directly to the import map" using an attribute is exotic, as in inconsistent with both how shadow roots work and with how the import map work. Usually shadow roots should not be able to implicitly affect what's outside of them, and this changes that: any shadow root can import something from a specifier and any shadow root can populate the style that corresponds to that specifier. I understand the performance-driven use case for this (sharing styles between custom elements that are nested deep in the DOM without duplicating them) but I think we need to find a design that's less radical in terms of breaking shadow DOM encapsulation. A solution I've brought up before for this was being able to somehow share a style based on an integrity digest, which guarantees that the key and values have some relationship between them, or alternatively having some sort of namespace for CSS modules that is overridable from inside shadow-roots (kind of like |
Adds support for Declarative CSS Modules via
<style type="module" specifier="specifiername">.shadowrootadoptedstylesheetsis handled in this PR: #12339(See WHATWG Working Mode: Changes for more details.)
Addresses #10673
/acknowledgements.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/obsolete.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )